GH-44042: [C++][Parquet] Limit num-of row-groups when building parquet for encrypted file #44043

mapleFU · 2024-09-10T11:59:09Z

Rationale for this change

Limit num-of row-groups when build parquet

What changes are included in this PR?

Limit num-of row-groups when build parquet

Are these changes tested?

No

Are there any user-facing changes?

No

GitHub Issue: [C++][Parquet] Limit RowGroup num to i16 during writing #44042

mapleFU · 2024-09-10T11:59:35Z

@wgtmac @pitrou would you mind take a look?

For testing, I don't know would it be too slow, if not I'd add one

github-actions · 2024-09-10T11:59:37Z

⚠️ GitHub issue #44042 has been automatically assigned in GitHub to PR creator.

pitrou · 2024-09-10T14:38:51Z

Is erroring out the right solution? The row group ordinal is optional and was introduced for encryption. Furthermore, it's not obvious that it should be unique. https://github.com/apache/parquet-format/blob/master/Encryption.md gives no guidelines as to ordinal generation.

@ggershinsky What is the intent here? Should ordinal be unique? But then, should the Parquet reader check for uniqueness? Otherwise a malicious file could still be crafted.

pitrou · 2024-09-10T15:01:16Z

Ok, the C++ reader always uses the physical row group number as the row group ordinal for AAD computation:

arrow/cpp/src/parquet/file_reader.cc

Lines 270 to 278 in b4b22a4

    
           constexpr auto kEncryptedRowGroupsLimit = 32767; 
        
           if (i > kEncryptedRowGroupsLimit) { 
        
             throw ParquetException("Encrypted files cannot contain more than 32767 row groups"); 
        
           } 
        
           CryptoContext ctx(col->has_dictionary_page(), row_group_ordinal_, 
        
                             static_cast<int16_t>(i), meta_decryptor, data_decryptor); 
        
           return PageReader::Open(stream, col->num_values(), col->compression(), properties_, 
        
                                   always_compressed, &ctx);

This begs the question: why is the row group ordinal stored in Thrift metadata if it's simply ignored when reading?

pitrou · 2024-09-10T15:01:57Z

And by the way, the same audit should be done for column and page ordinals.

wgtmac · 2024-09-10T15:11:04Z

Ok, the C++ reader always uses the physical row group number as the row group ordinal for AAD computation:

Isn't it a bug on the C++ side? It may produces wrong AAD when reading a Parquet file created by ParquetRewriter which binpacks several encrypted files.

EDIT: My bad. ParquetRewriter does not support binpack encrypted files yet.

pitrou · 2024-09-10T16:10:37Z

It may produces wrong AAD when reading a Parquet file created by ParquetRewriter which binpacks several encrypted files.

Hopefully it's not possible to concatenate encrypted files together? Otherwise one can create malicious data (replay attack).

ggershinsky · 2024-09-10T16:12:47Z

The number of row groups (columns, pages) should be limited for encrypted files only. I don't think this exception should be thrown for unencrypted files.

ggershinsky · 2024-09-10T16:31:41Z

It may produces wrong AAD when reading a Parquet file created by ParquetRewriter which binpacks several encrypted files.

Hopefully it's not possible to concatenate encrypted files together? Otherwise one can create malicious data (replay attack).

Yep, merging encrypted files is impossible, the AAD has a unique file id component.

pitrou · 2024-09-10T16:37:32Z

Yep, merging encrypted files is impossible, the AAD has a unique file id component.

It seems to be optional and disabled by default according to https://github.com/apache/parquet-format/blob/master/Encryption.md#441-aad-prefix ?

Regardless, my question was about ordinal reuse or shuffling. Should readers verify that ordinals correspond to the physical row group numbers? What is the intent exactly? The spec does not say how these should be handled.

ggershinsky · 2024-09-10T17:57:41Z

Yep, merging encrypted files is impossible, the AAD has a unique file id component.

It seems to be optional and disabled by default according to https://github.com/apache/parquet-format/blob/master/Encryption.md#441-aad-prefix ?

It's a different parameter, aad_file_unique, a mandatory part of the AAD suffix,
https://github.com/apache/parquet-format/blob/master/Encryption.md#442-aad-suffix
https://github.com/apache/parquet-format/blob/master/Encryption.md#52-crypto-structures

Regardless, my question was about ordinal reuse or shuffling. Should readers verify that ordinals correspond to the physical row group numbers? What is the intent exactly? The spec does not say how these should be handled.

The rg ordinals in thrift are a utility, which can be helpful for readers that split a single file into parallel reading threads. The readers can also run an rg loop/counter. The values will be the same, unless the file is tampered with. Note - the thrift footer is tamper-proof, so the rg ordinals are safe; but the row groups / pages can be shuffled by an attacker. Reading a page from a shuffled rg will throw an exception.
The situation is similar for column and page ordinals. The spec says "The suffix part of a module AAD protects against module swapping inside a file.", but doesn't go into smaller details because they can derived directly from the provided information.

pitrou · 2024-09-10T19:05:18Z

The spec says "The suffix part of a module AAD protects against module swapping inside a file.", but doesn't go into smaller details because they can derived directly from the provided information.

Since we're talking about a security feature, it would be much better if the spec gave clear guidelines instead of letting implementers do the necessary guesswork (and potentially make errors).

ggershinsky · 2024-09-11T08:46:10Z

I do agree the spec often feels laconic, not only here but in other places too. Expanding it, with ample explanation and reasoning, would result in a different document (implementation guide), much longer than the spec.
However, the spec details are sufficient for a secure implementation. In this example, a reader can use either a row group counter (the term "ordinal" has a well-defined meaning) or the thrift value; either one will support the intended security model. Still, since other ordinals (column and page) don't have a thrift duplicate, I can add a clarification text that makes this point explicit. Will send a patch.

mapleFU · 2024-09-23T03:16:44Z

@ggershinsky Would you mind add a patch to parquet-format? Then I can move forward here

ggershinsky · 2024-09-23T14:18:18Z

Sure, here it goes apache/parquet-format#453

mapleFU · 2024-09-29T17:41:39Z

apache/parquet-format#453 is merged and I understand this feature is for encryption. However, still this value is written to the metadata, should I:

Just throw like current impl when row-group ordinal is greater than i16::max
Not writing the ordinal when not encryption, so this check can be loosen

pitrou · 2024-09-29T18:52:56Z

Perhaps a quick check that other Parquet readers only use the row group ordinal for encrypted files?

curioustien · 2025-01-13T00:57:48Z

Perhaps a quick check that other Parquet readers only use the row group ordinal for encrypted files?

@mapleFU @pitrou I'm trying to pick up this task to ramp up on the parquet codebase. Upon checking the parquet java implementation of this ordinal field in RowGroup, I think there is a small bug/discrepancy in the current cpp implementation vs the java implementation.

For the java implementation, there is this check whether it's an encrypted file or not by checking the fileEncryptor pointer. If it's not an encrypted file, the rowGroupOrdinal value is -1 which I believe it means that Thrift won't set that field. Otherwise, we use the rowGroupOrdinal value which is incremented every time we flush the row group. Though, there is a small bug here in the java implementation that we use int instead of short for int16 ordinal variable. Plus, there are no checks here on integer overflow. In practice, people probably don't hit this limit on the number of row groups anyways, so this bug hasn't showed up yet.

For the cpp implementation, we don't really have a check for the encrypted file like the java implementation when we write row group ordinal (not that I've found, or maybe I'm missing something), so we always write this ordinal value even for unencrypted files. In my opinion, we should change the logic here to behave like the java implementation with an additional check on integer overflow.

What do you both think?

mapleFU · 2025-01-13T02:37:15Z

The idea LGTM

mapleFU · 2025-01-13T06:07:29Z

cpp/src/parquet/file_reader.cc

@@ -267,12 +267,13 @@ class SerializedRowGroup : public RowGroupReader::Contents {
    ARROW_DCHECK_NE(data_decryptor, nullptr);

    constexpr auto kEncryptedRowGroupsLimit = 32767;
-    if (i > kEncryptedRowGroupsLimit) {
+    if (ARROW_PREDICT_FALSE(row_group_ordinal_ > kEncryptedRowGroupsLimit)) {


The previous check actually checks the column_id, not the row-group ordinal.

mapleFU · 2025-01-13T06:08:03Z

cpp/src/parquet/file_writer.cc

@@ -359,14 +359,26 @@ class FileSerializer : public ParquetFileWriter::Contents {
    if (row_group_writer_) {
      row_group_writer_->Close();
    }
+    int16_t row_group_ordinal = 0;
+    if (num_row_groups_ < std::numeric_limits<int16_t>::max()) {
+      row_group_ordinal = static_cast<int16_t>(num_row_groups_);


This check is a bit ugly. Maybe we can also not write the row_group_ordinal for un-encryption files

Yes, I think there is no need to write it.

cpp/src/parquet/metadata.cc

mapleFU · 2025-01-13T06:25:59Z

@pitrou @wgtmac Sorry for delaying, would you mind take a look?

cpp/src/parquet/file_writer.cc

Co-authored-by: Antoine Pitrou <[email protected]>

mapleFU · 2025-01-13T11:24:58Z

cpp/src/parquet/metadata.cc


      if (ccmd.__isset.ENCRYPTION_WITH_COLUMN_KEY) {
        if (file_decryptor != nullptr && file_decryptor->properties() != nullptr) {
          // should decrypt metadata
          std::shared_ptr<schema::ColumnPath> path = std::make_shared<schema::ColumnPath>(
              ccmd.ENCRYPTION_WITH_COLUMN_KEY.path_in_schema);
-          std::string key_metadata = ccmd.ENCRYPTION_WITH_COLUMN_KEY.key_metadata;
+          const std::string& key_metadata = ccmd.ENCRYPTION_WITH_COLUMN_KEY.key_metadata;


Also wonder should we check row_group_ordinal >= 0 here? 🤔

I don't think it's necessary, any invalid row group ordinal will fail the AAD check when reading.

cpp/src/parquet/file_writer.cc

pitrou · 2025-01-13T13:58:57Z

cpp/src/parquet/metadata.cc


      if (ccmd.__isset.ENCRYPTION_WITH_COLUMN_KEY) {
        if (file_decryptor != nullptr && file_decryptor->properties() != nullptr) {
          // should decrypt metadata
          std::shared_ptr<schema::ColumnPath> path = std::make_shared<schema::ColumnPath>(
              ccmd.ENCRYPTION_WITH_COLUMN_KEY.path_in_schema);
-          std::string key_metadata = ccmd.ENCRYPTION_WITH_COLUMN_KEY.key_metadata;
+          const std::string& key_metadata = ccmd.ENCRYPTION_WITH_COLUMN_KEY.key_metadata;


I don't think it's necessary, any invalid row group ordinal will fail the AAD check when reading.

cpp/src/parquet/metadata.cc

wgtmac · 2025-01-13T14:53:12Z

cpp/src/parquet/file_reader.cc

-    constexpr auto kEncryptedRowGroupsLimit = 32767;
-    if (i > kEncryptedRowGroupsLimit) {
+    constexpr auto kEncryptedOrdinalLimit = 32767;
+    if (ARROW_PREDICT_FALSE(row_group_ordinal_ > kEncryptedOrdinalLimit)) {


I am wondering if this is the best place to enforce this. Is it possible to check these while creating the FileMetaData if encrypted?

It seems that if we don't request reading data from row_group_ordinal > 32767 and column_ordinal > 32767, we will even not notice that it is a malformed encrypted file.

Do you mean when contructing ColumnChunkMetaData::ColumnChunkMetaDataImpl ? What if the ENCRYPTION_WITH_COLUMN_KEY is not set but this is being encrypted?

conbench-apache-arrow · 2025-01-24T16:53:15Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit f4a63d4.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 339 possible false positives for unstable benchmarks that are known to sometimes produce them.

Limit num-of row-groups when build

e5c061d

mapleFU requested a review from wgtmac as a code owner September 10, 2024 11:59

github-actions bot added Component: Parquet Component: C++ awaiting review Awaiting review labels Sep 10, 2024

mapleFU added 2 commits January 13, 2025 11:55

Merge branch 'main' into limit-row-group-ordinal

df7cc42

Update the row_group_ordinal logic

2a95a77

mapleFU commented Jan 13, 2025

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jan 13, 2025

pitrou reviewed Jan 13, 2025

View reviewed changes

cpp/src/parquet/file_writer.cc Outdated Show resolved Hide resolved

pitrou mentioned this pull request Jan 13, 2025

GH-44042: [C++][Parquet] Limit num row groups with i16 #45231

Closed

mapleFU and others added 4 commits January 13, 2025 18:53

Apply suggestions from code review

e8adbf9

Co-authored-by: Antoine Pitrou <[email protected]>

Update cpp/src/parquet/metadata.cc

15b68e5

Co-authored-by: Antoine Pitrou <[email protected]>

Minor update

357d9d1

Some minor updates

f9f8d60

mapleFU commented Jan 13, 2025

View reviewed changes

pitrou reviewed Jan 13, 2025

View reviewed changes

wgtmac reviewed Jan 13, 2025

View reviewed changes

resolve comments

2817b5d

mapleFU changed the title ~~GH-44042: [C++][Parquet] Limit num-of row-groups when build parquet~~ GH-44042: [C++][Parquet] Limit num-of row-groups when build parquet for encrpty file Jan 14, 2025

mapleFU requested review from pitrou and wgtmac January 20, 2025 08:52

wgtmac changed the title ~~GH-44042: [C++][Parquet] Limit num-of row-groups when build parquet for encrpty file~~ GH-44042: [C++][Parquet] Limit num-of row-groups when building parquet for encrypted file Jan 24, 2025

wgtmac approved these changes Jan 24, 2025

View reviewed changes

pitrou approved these changes Jan 24, 2025

View reviewed changes

mapleFU merged commit f4a63d4 into apache:main Jan 24, 2025
37 checks passed

mapleFU removed the awaiting committer review Awaiting committer review label Jan 24, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-44042: [C++][Parquet] Limit num-of row-groups when building parquet for encrypted file #44043

GH-44042: [C++][Parquet] Limit num-of row-groups when building parquet for encrypted file #44043

mapleFU commented Sep 10, 2024 •

edited by github-actions bot

Loading

mapleFU commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

pitrou commented Sep 10, 2024 •

edited

Loading

pitrou commented Sep 10, 2024

pitrou commented Sep 10, 2024

wgtmac commented Sep 10, 2024 •

edited

Loading

pitrou commented Sep 10, 2024

ggershinsky commented Sep 10, 2024

ggershinsky commented Sep 10, 2024 •

edited

Loading

pitrou commented Sep 10, 2024

ggershinsky commented Sep 10, 2024

pitrou commented Sep 10, 2024

ggershinsky commented Sep 11, 2024

mapleFU commented Sep 23, 2024

ggershinsky commented Sep 23, 2024

mapleFU commented Sep 29, 2024

pitrou commented Sep 29, 2024

curioustien commented Jan 13, 2025

mapleFU commented Jan 13, 2025

mapleFU Jan 13, 2025

mapleFU Jan 13, 2025

pitrou Jan 13, 2025

mapleFU commented Jan 13, 2025

mapleFU Jan 13, 2025

pitrou Jan 13, 2025

pitrou Jan 13, 2025

wgtmac Jan 13, 2025

wgtmac Jan 13, 2025

mapleFU Jan 14, 2025

conbench-apache-arrow bot commented Jan 24, 2025

GH-44042: [C++][Parquet] Limit num-of row-groups when building parquet for encrypted file #44043

GH-44042: [C++][Parquet] Limit num-of row-groups when building parquet for encrypted file #44043

Conversation

mapleFU commented Sep 10, 2024 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

mapleFU commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

pitrou commented Sep 10, 2024 • edited Loading

pitrou commented Sep 10, 2024

pitrou commented Sep 10, 2024

wgtmac commented Sep 10, 2024 • edited Loading

pitrou commented Sep 10, 2024

ggershinsky commented Sep 10, 2024

ggershinsky commented Sep 10, 2024 • edited Loading

pitrou commented Sep 10, 2024

ggershinsky commented Sep 10, 2024

pitrou commented Sep 10, 2024

ggershinsky commented Sep 11, 2024

mapleFU commented Sep 23, 2024

ggershinsky commented Sep 23, 2024

mapleFU commented Sep 29, 2024

pitrou commented Sep 29, 2024

curioustien commented Jan 13, 2025

mapleFU commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Jan 24, 2025

mapleFU commented Sep 10, 2024 •

edited by github-actions bot

Loading

pitrou commented Sep 10, 2024 •

edited

Loading

wgtmac commented Sep 10, 2024 •

edited

Loading

ggershinsky commented Sep 10, 2024 •

edited

Loading